Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add some more g2 icons #21825

Merged
merged 25 commits into from
Apr 30, 2020
Merged

Add some more g2 icons #21825

merged 25 commits into from
Apr 30, 2020

Conversation

jameskoster
Copy link
Contributor

In this PR I'm proposing some new icons, and some updates to existing ones.

Updates

The current cog icon is awkwardly rendered - its 'body' isn't quite circular. I've tidied this up.

The plugins and help icons have seemingly been lifted directly out of Dashicons, so don't match the rest of the set. I've used existing g2 icons as inspiration for updating these.

I also updated the alignment of the arrow-left icon so that it is perfectly centred.

beforeafter

New icons

Additionally, I'm proposing some new icons for home, bar chart, megaphone, people, tool, and support. Most of these exist in Dashicons, but support is new.

new icons

@jasmussen @pablohoneyhoney

sirreal and others added 12 commits April 22, 2020 16:42
[WordPress#21613](WordPress#21613) "hid"
TypeScript emitted type declarations for `@wordpress/element` when it
was found to conflict with 3rd party type declarations on
DefinitelyTyped.

Disabling `@wordpress/element` cascaded to require disabling type
checking for `@wordpress/icons` and `@wordpress/primitives`, two
dependent packages.

The lint-staged flow is slightly different from the full
`build:package-types` script in that it attempts to do a minimal build
in the interest of speed. This flow checks for the presence of a
`tsconfig.json` file in the root of the package with changes and runs
its package build.

While removing the affected packages from the main tsconfig.json did
remove them from the primary build, changes to primitives and icons
packages will fail because they cannot find the `@wordpress/element`
types as was intended in WordPress#21613.

By renaming the package tsconfig.json files, they are excluded from the
lint-staged typechecking as well.
@jasmussen
Copy link
Contributor

Nice work. The before/afters look good. Did we really not have the help icon before? I noticed btw. that the lowercase I info icon we might need to update that one to better match the new question mark in stem width. Separate thing to look at.

The plug looks decent. I never loved that metaphor, but I can't think of a better one at the moment. And the verticality and outline-stroke adds some clarity that the diagonal one lacked. But what's important then is getting the name right, because "plugin" could be either a plug or a puzzle piece or any other metaphor. Material calls them "power", how about that?

Screenshot 2020-04-24 at 09 56 24

Here's the material extension icon:

Screenshot 2020-04-24 at 10 06 43

As for the new icons. I love the stylishness of the "House" icon. It's possibly too stylish, but I'd rather we start with that and pull back when we need to, rather than default to less. Bar charts and megaphones also look good, people looks great.

Tool is a good name, I'm unsure about the wrench. It both feels too small and too dark at the same time. I wonder if there's a different metaphor that's more legible yet has potential for stylishness. I personally really love the simplicity of this pen, and the line it draws:

Screenshot 2020-04-24 at 10 00 34

These two both feel more balanced and clear, although of course they have nothing to do with wrenches:

Screenshot 2020-04-24 at 10 06 00

In the wrench, the hexagonal bit in the "head" is perhaps the most distinct part. Is there room to emphasize that? Could we even do with just a lugnut? What if instead of a tool we looked at the section it's intended for and found a different metaphor?

This one for "tune" is also interesting as an alternative for settings, but perhaps not a 1:1 with the tool goal you're trying to accomplish:

Screenshot 2020-04-24 at 10 05 41

Support is tricky. I don't mind the lifesaver there, though it is a little dark with the dual strokes. Can it work in a solid version?

Important to all of these is to get the names right. Good to go with the metaphor rather than the purpose, most of the time, as that lets us more easily revisit and refine them if we need to.

@jameskoster
Copy link
Contributor Author

jameskoster commented Apr 24, 2020

Material calls them "power", how about that?

I have no problem with "power" as a name, but the "plugins" icon already exists, so we'd have to leave it to maintain backwards compatibility. If we added a separate "power" icon with the plug metaphor, that would result in having two icons in the set that serve the same purpose, but with different names. Doesn't sound like a great situation. Seems simplest to just update the "plugins" icon to visually match the rest of the set, then perhaps add a separate "extensions" icon in the future.

For the tool, I did originally try some other variants:

tool

What if instead of a tool we looked at the section it's intended for and found a different metaphor?

We intend to use it for our "tools" menu in WooCommerce. It contains features like importers, system status reports, and other utilities like permission resets, thumbnail regeneration, etc. If we'd like to explore something more stylish, "robot" or "automation" might be an avenue to go down.

A lugnut / bolt could work, but I fear it's too close to the "cog" icon:

Reversing the lifesaver just makes it feel darker, and less like a lifesaver, to me at least:

@jasmussen
Copy link
Contributor

Agreed on the lugnut and lifesaver. But we need to crack that wrench.

This is obv. a lightbulb with a wrench in it, which won't work:

Screenshot 2020-04-24 at 11 48 16

But from that one, I like that:

  • the wrench is vertical
  • it's big
  • and you can see some of the hexagonal lugnut style inside the wrench

I wonder if we can move towards that, somehow?

@jameskoster
Copy link
Contributor Author

jameskoster commented Apr 24, 2020

A chunkier / vertical wrench works ok. Personally I think it's a bit too chunky when filled, but an outlined version is decent.

I played around with the robot / automation idea too, one a bit more "out there", and one inspired by the pattern we're using for "people":

@jasmussen
Copy link
Contributor

Progress! Can you try the wrench not being rounded at the bottom? Or at least rounded like the 2nd one in the two above= And do you think we can get the hexagons going on the outside as well?

Something like this perhaps:

Screenshot 2020-04-24 at 13 02 01

And maybe it does have to be diagonal after all, to not feel too small.

@jameskoster
Copy link
Contributor Author

I think that diagonal is the way to go. It's immediately familiar in that orientation.

I also think the outlined version works best. The filled versions are just too chunky at these dimensions - unless we go with something closer to the original proposal. I hexagonalised that for a comparison.

@jameskoster
Copy link
Contributor Author

I just noticed that empty / half-filled versions of the star icon exist in the g2 set, but they seem to have been transplanted from Dashicons. I'm going to update those in this PR as well.

Top row is g2 - based on the previously approved star icon. Bottom row is the current implemented icons.

Replaces old Dashicons stars with g2 equivalents
@jasmussen
Copy link
Contributor

Much nicer stars, thanks Jay. Those are 1.5px strokes too, right? Big improvement.

How do you feel about the innermost radius of the stars? It may be a non issue when shown at 1x but at a glance it feels slightly too "bloated" for want of a better term.

Here's a fun star generator. With an outer radius of 100 and an inner of 38, it looks geometrically star-like:

Screenshot 2020-04-24 at 16 24 38

This one looks too sharp.

With an outer 100 and inner 60, it's too childish:

Screenshot 2020-04-24 at 16 27 12

This is 100/46 — that seems like it balances the two extremes maybe?

Screenshot 2020-04-24 at 16 27 44

@jameskoster
Copy link
Contributor Author

I don't have strong feelings. I mocked up the current star shape (left) vs a 100/46 star (right):

Perhaps the 100/46 star is a little cramped with the 1.5px stroke?

Here are some more comparisons:

100/50 seems like a nice compromise :)

@jasmussen
Copy link
Contributor

Thanks for such a fast response! Let's indeed try 100/50.

Comment on lines 9 to 11
fill-rule="evenodd"
d="M9.706 8.646a.25.25 0 01-.188.137l-4.626.672a.25.25 0 00-.139.427l3.348 3.262a.25.25 0 01.072.222l-.79 4.607a.25.25 0 00.362.264l4.138-2.176a.25.25 0 01.233 0l4.137 2.175a.25.25 0 00.363-.263l-.79-4.607a.25.25 0 01.072-.222l3.347-3.262a.25.25 0 00-.139-.427l-4.626-.672a.25.25 0 01-.188-.137l-2.069-4.192a.25.25 0 00-.448 0L9.706 8.646zM12 7.39l-.948 1.921a1.75 1.75 0 01-1.317.957l-2.12.308 1.534 1.495c.412.402.6.982.503 1.55l-.362 2.11 1.896-.997a1.75 1.75 0 011.629 0l1.895.997-.362-2.11a1.75 1.75 0 01.504-1.55l1.533-1.495-2.12-.308a1.75 1.75 0 01-1.317-.957L12 7.39z"
clip-rule="evenodd"
Copy link
Member

@aduth aduth Apr 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are console warnings here:

Warning: Invalid DOM property fill-rule. Did you mean fillRule

Warning: Invalid DOM property clip-rule. Did you mean clipRule

The warnings should be pretty self-explanatory in their solutions, I hope 😄

See also: https://reactjs.org/docs/dom-elements.html#all-supported-html-attributes

@jameskoster
Copy link
Contributor Author

jameskoster commented Apr 27, 2020

Updated the tool icon after some conversations with Joen:

@jasmussen
Copy link
Contributor

Good work here!

My general feeling towards adding icons, especially when there are hard-work PRs like this one, is that we should be liberal in adding them. So long as:

  • the icons have good generic descriptive icon names, describing the icon, not the function you intend for it
  • good general metaphors
  • good baseline icon design that generally fits the g2 style

With the work you've done so far, I feel like we've reached that point with this PR! So from a design POV, 👍 👍

I have a bit on my plate today, so I can't code-review this. But feel free to get this sanity checked elsewhere and merged. Thanks Jay!

@@ -113,7 +118,6 @@ export { default as search } from './library/search';
export { default as separator } from './library/separator';
export { default as share } from './library/share';
export { default as shortcode } from './library/shortcode';
export { default as star } from './library/star';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this icon ever published as part of a stable WordPress or Gutenberg release?

If so, we can't simply remove it, since there's a backwards-compatibility commitment.

If it's only ever been published in the plugin, we can deprecate it after two versions.

See:

If it was published in a stable WordPress release, then it must be kept forever.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, or is this a unique package in how it's not actually enqueued in the editor? I guess at the very least, if it's only ever intended to be consumed as a package, we'd still want to include a CHANGELOG entry about the removal being a breaking change.

See: https://github.com/WordPress/gutenberg/blob/master/packages/README.md#maintaining-changelogs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was added, as an oversight here. star-filled already existed (and was identical in it's intended application), so star is extraneous. Let me know how you think we should proceed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess if it isn't exposed directly on the window global, and it never landed in a published version of the package (verified), then no additional action should be necessary.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From purely a code perspective, it looks good 👍

packages/icons/CHANGELOG.md Outdated Show resolved Hide resolved
packages/icons/package.json Outdated Show resolved Hide resolved
@aduth
Copy link
Member

aduth commented Apr 30, 2020

Thanks for the latest updates with the CHANGELOG 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants